QVAC-21396 fix(mtmd/qwen3vl): grid selection rewrite + --image-max-tiles override#175
Conversation
yingying0906
changed the title
yingying0906
force-pushed
the
tetherto/fix/qwen3vl-grid-select
branch
2 times, most recently
from
1c1311d to
31a107e
Compare
github-actions
Bot
added
documentation
yingying0906
force-pushed
the
tetherto/fix/qwen3vl-grid-select
branch
from
31a107e to
fcce509
Compare
| loader.load_hparams(ctx_vision->model, CLIP_MODALITY_VISION); | ||
| // apply CLI overrides after load_hparams so GGUF defaults don't clobber them | ||
| if (ctx_params.image_max_tiles != -1) { | ||
| ctx_vision->model.hparams.preproc_max_tiles = ctx_params.image_max_tiles; |
There was a problem hiding this comment.
[security] image_max_tiles is unvalidated at this library boundary → potential OOM/DoS.
The [1,256] bound is enforced only in common/arg.cpp (the CLI/env path). But clip_context_params/mtmd_context_params are the public MTMD_API surface that native bindings populate directly (e.g. this monorepo's packages/llm-llamacpp). Here the override only tests != -1 and assigns straight into hparams.preproc_max_tiles. A large value then reaches fitting.reserve((size_t)(max_tiles * std::log(max_tiles))) in mtmd-image.cpp — an INT_MAX-ish value requests hundreds of GB and throws std::bad_alloc, and the per-image preprocess() call in mtmd.cpp's tokenize path has no surrounding try/catch, so the whole process can crash.
The correct pattern already exists in this same PR two functions up (the GGUF-read path clamps preproc_max_tiles to [1,256] with a warning). Please re-validate here too, ideally via a shared constant reused by both sites.
There was a problem hiding this comment.
Fixed. The override now clamps to [1, CLIP_PREPROC_MAX_TILES_LIMIT] (256) at this boundary before assigning, with a warning, so a binding that populates clip_context_params directly can no longer drive the grid-fitting reserve into bad_alloc. Uses the same shared constant as the GGUF-read path.
| LOG_INF("%s: preproc_max_tiles: %d (custom value)\n", __func__, ctx_vision->model.hparams.preproc_max_tiles); | ||
| // models with a min-tiles floor (e.g. InternVL) would produce an empty candidate | ||
| // set if the override drops max below min; clamp min so the invariant holds. | ||
| if (ctx_vision->model.hparams.preproc_min_tiles > ctx_vision->model.hparams.preproc_max_tiles) { |
There was a problem hiding this comment.
[correctness] This override (and the min-tiles clamp) is a silent no-op for InternVL.
InternVL builds hparams.image_res_candidates exactly once, inside load_hparams (set_internvl_dhr_res_candidates, called only from that switch case). clip_init applies this --image-max-tiles override after load_hparams, and nothing rebuilds the candidate list afterward. InternVL's tiling (mtmd_image_preprocessor_llava_uhd::get_slice_instructions) reads only image_res_candidates — it never reads preproc_min_tiles/preproc_max_tiles live. So for InternVL, mutating these fields here has zero effect on tiling, yet we still log preproc_max_tiles: N (custom value), and this min-tiles clamp is dead code (the "empty candidate set" it guards against can't occur, since the candidates are already fixed). This contradicts the flag's own help text.
(It works correctly for Qwen3VL, which reads preproc_max_tiles at preprocessor construction, post-override.)
Fix options: rebuild image_res_candidates after applying the override for candidate-list-based preprocessors, or scope the flag + docs to Qwen3VL only.
There was a problem hiding this comment.
Fixed. Scoped the override to PROJECTOR_TYPE_QWEN3VL which is the only preprocessor that reads preproc_max_tiles live. Non-Qwen3VL projectors now get a "only supported for Qwen3VL; ignoring" warning instead of a silent no-op + misleading (custom value) log. Removed the min-tiles clamp entirely: it only guarded InternVL, whose candidate list is frozen in load_hparams, so it was dead code.
| "for your model size (Qwen3VL defaults to 4, too low for 8B+ variants)", | ||
| [](common_params & params, int value) { | ||
| if (value < 1) { throw std::invalid_argument("--image-max-tiles must be >= 1"); } | ||
| if (value > 256) { throw std::invalid_argument("--image-max-tiles must be <= 256"); } |
There was a problem hiding this comment.
[consistency] The [1,256] bound is a hardcoded literal duplicated across two sites, and one enforcement site is missing.
This is one of three independent hardcodings of 256: here (CLI), the GGUF-read validation in clip.cpp (preproc_max_tiles < 1 || > 256), and it is absent at the clip_init override site (ctx_vision->model.hparams.preproc_max_tiles = ctx_params.image_max_tiles;) which is the actual library boundary embedders hit. Suggest extracting a shared constexpr (e.g. IMAGE_MAX_TILES_LIMIT = 256) and using it at all three places so the invariant can't drift and the override path gets the bound for free.
There was a problem hiding this comment.
The override site now enforces the bound via a shared constexpr CLIP_PREPROC_MAX_TILES_LIMIT in clip-impl.h, reused at both clip.cpp sites. Left the literal here on purpose: arg.cpp is in common/ (includes only common.h) and can't reach clip-impl.h without coupling the common lib to mtmd internals for one int. Since the safety-critical bound is now at the library boundary, this CLI check is just a friendly early-reject, a drift here can't cause the OOM.
| const char * model_name = | ||
| model.proj_type == PROJECTOR_TYPE_QWEN2VL ? "Qwen2VL" : | ||
| model.proj_type == PROJECTOR_TYPE_QWEN25VL ? "Qwen2.5VL" : "Qwen3VL"; | ||
| LOG_WRN("%s: %s large ViT detected (n_head=%d, n_layer=%d) but " |
There was a problem hiding this comment.
[consistency] This warning tells the user to --image-max-tiles for projectors where the flag is inert.
This block lives in a switch case shared by PROJECTOR_TYPE_QWEN2VL, QWEN25VL, and QWEN3VL, and the ternary above names "Qwen2VL"/"Qwen2.5VL". But per mtmd.cpp's preprocessor-selection switch, QWEN2VL/QWEN25VL use mtmd_image_preprocessor_dyn_size, which never reads preproc_max_tiles — tiling is Qwen3VL-only. So suggesting --image-max-tiles to Qwen2/2.5VL users is misleading (the flag does nothing there). Either scope this default/validation/warning block to PROJECTOR_TYPE_QWEN3VL, or drop the --image-max-tiles suggestion from the non-Qwen3VL branches.
There was a problem hiding this comment.
The "set --image-max-tiles" suggestion is now gated to PROJECTOR_TYPE_QWEN3VL; Qwen2/2.5VL (dyn_size) get the same large-ViT warning without the flag suggestion, since the flag is inert for them.
…e to Qwen3VL Addresses review on PR #175: - Clamp the preproc_max_tiles override to [1,256] in clip_init. Bindings populate clip_context_params directly and bypass the CLI's range check; an unbounded value reached the O(max_tiles*log max_tiles) grid-fitting reserve in mtmd-image.cpp and could request hundreds of GB -> std::bad_alloc -> process crash. - Extract a shared CLIP_PREPROC_MAX_TILES_LIMIT (clip-impl.h) and use it at both the GGUF-read and override sites so the bound can't drift. - Scope the override to PROJECTOR_TYPE_QWEN3VL (the only preprocessor that reads the field live); warn and ignore for other projectors. Drop the dead InternVL min-tiles clamp (InternVL reads a candidate list frozen in load_hparams, not the live field). - Gate the "set --image-max-tiles" suggestion in the shared Qwen2/2.5/3 case to Qwen3VL only; the flag is inert for Qwen2/2.5VL (dyn_size). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e to Qwen3VL Addresses review on PR #175: - Clamp the preproc_max_tiles override to [1,256] in clip_init. Bindings populate clip_context_params directly and bypass the CLI's range check; an unbounded value reached the O(max_tiles*log max_tiles) grid-fitting reserve in mtmd-image.cpp and could request hundreds of GB -> std::bad_alloc -> process crash. - Extract a shared CLIP_PREPROC_MAX_TILES_LIMIT (clip-impl.h) and use it at both the GGUF-read and override sites so the bound can't drift. - Scope the override to PROJECTOR_TYPE_QWEN3VL (the only preprocessor that reads the field live); warn and ignore for other projectors. Drop the dead InternVL min-tiles clamp (InternVL reads a candidate list frozen in load_hparams, not the live field). - Gate the "set --image-max-tiles" suggestion in the shared Qwen2/2.5/3 case to Qwen3VL only; the flag is inert for Qwen2/2.5VL (dyn_size).
yingying0906
force-pushed
the
tetherto/fix/qwen3vl-grid-select
branch
from
5f0ed67 to
4847e7c
Compare
…e to Qwen3VL Addresses review on PR #175: - Clamp the preproc_max_tiles override to [1,256] in clip_init. Bindings populate clip_context_params directly and bypass the CLI's range check; an unbounded value reached the O(max_tiles*log max_tiles) grid-fitting reserve in mtmd-image.cpp and could request hundreds of GB -> std::bad_alloc -> process crash. - Extract a shared CLIP_PREPROC_MAX_TILES_LIMIT (clip-impl.h) and use it at both the GGUF-read and override sites so the bound can't drift. - Scope the override to PROJECTOR_TYPE_QWEN3VL (the only preprocessor that reads the field live); warn and ignore for other projectors. Drop the dead InternVL min-tiles clamp (InternVL reads a candidate list frozen in load_hparams, not the live field). - Gate the "set --image-max-tiles" suggestion in the shared Qwen2/2.5/3 case to Qwen3VL only; the flag is inert for Qwen2/2.5VL (dyn_size).
yingying0906
force-pushed
the
tetherto/fix/qwen3vl-grid-select
branch
from
4847e7c to
80de190
Compare
Phase A overlay validation for fabric PR tetherto/qvac-fabric-llm.cpp#175 (grid selection rewrite + --image-max-tiles override). - add shared overlay port packages/ports/qvac-fabric (REF pinned to fabric commit 1c1311da7, version 9341.2.0) - wire overlay-ports ["../ports"] into all 6 consumer vcpkg-configuration.json Registry baselines and consumer version>= constraints untouched; overlay bypasses version resolution.
Phase A overlay validation for fabric PR tetherto/qvac-fabric-llm.cpp#175 (grid selection rewrite + --image-max-tiles override). - add shared overlay port packages/ports/qvac-fabric (REF pinned to fabric commit 1c1311da7, version 9341.2.0) - wire overlay-ports ["../ports"] into all 6 consumer vcpkg-configuration.json Registry baselines and consumer version>= constraints untouched; overlay bypasses version resolution.
…e to Qwen3VL Addresses review on PR #175: - Clamp the preproc_max_tiles override to [1,256] in clip_init. Bindings populate clip_context_params directly and bypass the CLI's range check; an unbounded value reached the O(max_tiles*log max_tiles) grid-fitting reserve in mtmd-image.cpp and could request hundreds of GB -> std::bad_alloc -> process crash. - Extract a shared CLIP_PREPROC_MAX_TILES_LIMIT (clip-impl.h) and use it at both the GGUF-read and override sites so the bound can't drift. - Scope the override to PROJECTOR_TYPE_QWEN3VL (the only preprocessor that reads the field live); warn and ignore for other projectors. Drop the dead InternVL min-tiles clamp (InternVL reads a candidate list frozen in load_hparams, not the live field). - Gate the "set --image-max-tiles" suggestion in the shared Qwen2/2.5/3 case to Qwen3VL only; the flag is inert for Qwen2/2.5VL (dyn_size).
yingying0906
force-pushed
the
tetherto/fix/qwen3vl-grid-select
branch
from
80de190 to
dfc03ff
Compare
DmitryMalishev
left a comment
DmitryMalishev
left a comment
There was a problem hiding this comment.
@yingying0906 Looks good to me overall. Couple corner case concerns worth to check and fix:
1. Flag silently dead for InternVL. --image-max-tiles is documented as generic ("multi-tile vision models") but has no effect on InternVL. InternVL bakes its resolution candidates from preproc_min/max_tiles into hparams.image_res_candidates during load_hparams (clip.cpp:1290, 2730–2748); the override in clip_init (clip.cpp:2767) mutates preproc_max_tiles afterwards but never rebuilds that candidate list, so preprocessing keeps using the old budget. Worse, the adjacent comment ("models with a min-tiles floor (e.g. InternVL) … clamp min so the invariant holds") reads as if InternVL were handled, giving reviewers false confidence. Either recompute the candidates after the override or scope the flag/docs to Qwen3-VL.
2. No API-side validation. Range validation exists only at the CLI (arg.cpp enforces 1–256). The mtmd_context_params.image_max_tiles / clip_context_params.image_max_tiles API path — which is what the QVAC addons actually use — applies any value other than -1 unchecked. 0 sets preproc_max_tiles = 0, which makes the qwen3vl grid loop never run and silently forces single-tile mode forever; a huge value allows unbounded tile counts (memory blow-up during preprocessing). The GGUF-read path got a [1,256] sanity check in this same PR — the override path should get the identical check in clip_init.
3. Two conflicting "defaults". The default flipped from batched to sequential, but only for callers going through mtmd_context_params_default(). The raw enum still has 0 = batched (clip.h: "0=batched, 1=sequential (default)"), so any consumer that zero-initializes the params struct silently gets batched — the unverified path from C1. A C API where "default" and "zero value" disagree is a long-term footgun; consider renumbering so 0 = sequential, or at minimum call this out loudly in mtmd.h.
…t the default Addresses PR #175 review (#3): the shipped enum is 0=batched while the default mode is sequential, so a zero-initialized clip_context_params/mtmd_context_params silently selects the batched (unverified ne[3]) path instead of the intended default. Renumbering to 0=sequential would be the clean fix, but the values are already released as a string API ("0"/"1"/"2" in consumers), so renumbering would break existing callers. Instead, document the trap prominently in clip.h and mtmd.h and point callers at mtmd_context_params_default() (which sets sequential).
|
Thanks for the comments and all three addressed. (1) InternVL no-op + misleading comment — resolved in (2) no API-side validation — resolved in the same commit. (3) 0=batched vs sequential default — agreed it's a real footgun. For now I've documented it in |
| // Only the Qwen3VL preprocessor reads preproc_max_tiles live; Qwen2/2.5VL use dyn_size | ||
| // and InternVL uses a candidate list already frozen in load_hparams, so the override is | ||
| // inert for them — apply (and validate) it only where it takes effect. | ||
| if (ctx_params.image_max_tiles != -1) { |
There was a problem hiding this comment.
[compatibility] image_max_tiles zero-initializes to an explicit override.
image_max_tiles uses -1 as the unset sentinel, but this is a new public struct field. Existing bindings/direct C API callers that zero-initialize clip_context_params/mtmd_context_params will pass 0 unless they are updated. This branch treats any value except -1 as an override, then clamps 0 to 1, so those callers silently force Qwen3VL to one max tile instead of using the GGUF/model default.
The CLI path is fine because common_params initializes image_max_tiles = -1, but public API consumers can regress in model quality: multi-tile preprocessing is effectively disabled for large/high-resolution images, especially hurting OCR/doc/screenshot cases where tiling preserves small local details. Consider treating 0 as unset at this library boundary, or adding/updating a public default initializer that bindings use before passing params into clip_init().
There was a problem hiding this comment.
Fixed. image_max_tiles now treats only a positive value as an explicit override, so both -1 and 0 fall through to the GGUF/model default. A zero-initialized clip_context_params / mtmd_context_params (bindings / direct C API callers that never set the field) keeps the model default instead of being clamped to single-tile. Dropped the dead lower clamp, kept the upper [1,256] bound. Updated the field docs in clip.h and mtmd.h to note -1 or 0 = default, only positive overrides.
Rewrite Qwen3VL multi-tile grid selection: enumerate CxR grids that downscale the image in both dimensions (col*tile_px<=nx, row*tile_px<=ny), exclude 1x1 (equivalent to dyn_size), then pick within a ratio-error tolerance band by max tile count and min log-ratio error. Fall back to dyn_size when no grid fits. Add a global overview thumbnail at entries[0]. Add the --image-max-tiles CLI/env override (1..256), applied after load_hparams so GGUF defaults don't clobber it, and clamp preproc_min_tiles so a low override can't produce an empty candidate set on InternVL-family models. Guard the log-ratio division against zero-dimension images. Hardening: - Validate clip.vision.preproc_max_tiles read from the GGUF: clamp to [1,256] (the --image-max-tiles range) and fall back to 4 with a warning, matching the InternVL branch. A crafted/corrupt value could otherwise drive an oversized fitting.reserve() and OOM on load or first inference. - Size fitting.reserve() to max_tiles*ln(max_tiles) (the actual candidate count) instead of max_tiles*2, which undershot ~3x at the top of the range. - Fail a zero-dimension image gracefully (LOG_ERR + return false) instead of GGML_ASSERT, so one malformed upload fails its own request rather than aborting the whole server. Callers already treat false as a per-request error. - Doc/log consistency: reflow --image-max-tiles help to the sibling --image-* style, add the --image-tile-mode/--image-max-tiles rows to the cli/server READMEs, and log preproc_max_tiles as "(custom value)" when overridden.
Replace the per-tile attention loop (a ggml_new_tensor_2d accumulator with ggml_set_2d writes and per-tile ggml_view_1d position slices, one rope pair per tile per layer) with zero-copy 3D views over the batched QKV buffer: a single rope pair runs on the collapsed n_pos*batch_size sequence, then 4D views feed one block-diagonal build_attn per layer. Fewer graph nodes and tensor descriptors, scaling with n_layer instead of n_layer*batch_size. Fix the M-RoPE position layout for batch_size>1: the mrope kernel reads section s of token i at positions[s*N + i] (N = n_pos*batch_size), so the buffer must be section-major across the full batched sequence, not tile- major. The old layout mis-indexed every section beyond the first tile, encoding (y,y) instead of (y,x). batch_size==1 (the sequential path) is unchanged. Default --image-tile-mode to sequential.
…e to Qwen3VL Addresses review on PR #175: - Clamp the preproc_max_tiles override to [1,256] in clip_init. Bindings populate clip_context_params directly and bypass the CLI's range check; an unbounded value reached the O(max_tiles*log max_tiles) grid-fitting reserve in mtmd-image.cpp and could request hundreds of GB -> std::bad_alloc -> process crash. - Extract a shared CLIP_PREPROC_MAX_TILES_LIMIT (clip-impl.h) and use it at both the GGUF-read and override sites so the bound can't drift. - Scope the override to PROJECTOR_TYPE_QWEN3VL (the only preprocessor that reads the field live); warn and ignore for other projectors. Drop the dead InternVL min-tiles clamp (InternVL reads a candidate list frozen in load_hparams, not the live field). - Gate the "set --image-max-tiles" suggestion in the shared Qwen2/2.5/3 case to Qwen3VL only; the flag is inert for Qwen2/2.5VL (dyn_size).
…t the default Addresses PR #175 review (#3): the shipped enum is 0=batched while the default mode is sequential, so a zero-initialized clip_context_params/mtmd_context_params silently selects the batched (unverified ne[3]) path instead of the intended default. Renumbering to 0=sequential would be the clean fix, but the values are already released as a string API ("0"/"1"/"2" in consumers), so renumbering would break existing callers. Instead, document the trap prominently in clip.h and mtmd.h and point callers at mtmd_context_params_default() (which sets sequential).
Zero-initialized clip_context_params/mtmd_context_params pass 0, which was clamped to 1 and silently forced single-tile preprocessing. Only a positive value now overrides the GGUF/model default.
yingying0906
force-pushed
the
tetherto/fix/qwen3vl-grid-select
branch
from
5707f9b to
bfa536c
Compare
…e to Qwen3VL Addresses review on PR tetherto#175: - Clamp the preproc_max_tiles override to [1,256] in clip_init. Bindings populate clip_context_params directly and bypass the CLI's range check; an unbounded value reached the O(max_tiles*log max_tiles) grid-fitting reserve in mtmd-image.cpp and could request hundreds of GB -> std::bad_alloc -> process crash. - Extract a shared CLIP_PREPROC_MAX_TILES_LIMIT (clip-impl.h) and use it at both the GGUF-read and override sites so the bound can't drift. - Scope the override to PROJECTOR_TYPE_QWEN3VL (the only preprocessor that reads the field live); warn and ignore for other projectors. Drop the dead InternVL min-tiles clamp (InternVL reads a candidate list frozen in load_hparams, not the live field). - Gate the "set --image-max-tiles" suggestion in the shared Qwen2/2.5/3 case to Qwen3VL only; the flag is inert for Qwen2/2.5VL (dyn_size).
…t the default Addresses PR tetherto#175 review (tetherto#3): the shipped enum is 0=batched while the default mode is sequential, so a zero-initialized clip_context_params/mtmd_context_params silently selects the batched (unverified ne[3]) path instead of the intended default. Renumbering to 0=sequential would be the clean fix, but the values are already released as a string API ("0"/"1"/"2" in consumers), so renumbering would break existing callers. Instead, document the trap prominently in clip.h and mtmd.h and point callers at mtmd_context_params_default() (which sets sequential).
4 participants
Summary
Rewrites Qwen3-VL multi-tile grid selection, adds an
--image-max-tilesoverride, collapses the batched-attention loop, and makes sequential the default tile mode.mtmd-image.cpp): deterministic best-grid search over downscaling grids; falls back todyn_sizewhen no grid fits. Adds a global overview thumbnail atentries[0].--image-max-tilesoverride (arg.cpp,common.h,mtmd.{h,cpp}, server/cli wiring): user-facing cap on tiles per image (1..256), applied afterload_hparamsso GGUF defaults don't clobber it.clip.cpp,clip.h,qwen3vl.cpp): zero-copy 3D views over the batched QKV buffer instead of a per-tile loop; fixes the M-RoPE section-major position layout forbatch_size>1; defaults--image-tile-modeto sequential.Grid selection mechanism
For an
nx × nyimage with per-tile sidetile_px(from model hparams) and a tile budgetmax_tiles. Each image is processed independently:col × rowgrid withcol*row ≤ max_tiles, excluding1×1, keeping only grids that downscale in both axes (col*tile_px ≤ nxandrow*tile_px ≤ ny) — upscaling adds no real detail.ratio_err = |log(nx/ny) − log(col/row)|. Log space makes2:1and1:2symmetric around square. This is the per-axis stretch the resize-to-canvas step will impose.dyn_size. If no grid downscales, usedyn_size(aspect-correct downscale at native detail, single tile).RATIO_ERR_TOLERANCE(0.2) of the bestratio_errand pick the most tiles (more spatial detail), tie-breaking on smallerratio_err. The band stops a high-tile-count grid from winning when a near-perfect-ratio, fewer-tile grid exists (e.g.2×2beating2×1on a 2:1 image and squashing it 2×).col × rowcanvas, prepend atile_px × tile_pxglobal overview thumbnail (recovers text split across tile seams), then crop intocol × rowtiles packed row-major. Tiles are encoded per--image-tile-mode:sequential(default, one-by-one),batched(one collapsed attention pass), ordisabled(single tile).Performance
Candidate (this PR) vs published
@qvac/llm-llamacpp0.31.0. Qwen3.5-0.8B, deterministic at temp 0 seed 42,image_max_tokens=4096. Thedisc-ocrset is 9 real getomni-ai OCR docs scaled aspect-preserved into the band where baseline falls back to dyn_size and candidate tiles.Addon benchmark: disc-ocr run
All rows from the same run. Speed, lower is faster. Desktop reports mmproj vision-encode; mobile reports TTFT.
Accuracy, CER, lower is better.
Raw fabric CLI on-device, per-image mean encode
Test plan
qwen3-5-image-tile-mode-tokens: batched token-count must match sequential, or a backend mis-iteratesne[3]). CI confirmation still pending — the mobile leg that covers Metal / Vulkan (Android) / OpenCL (Adreno) hit a Device Farm scheduling flake and needs a re-run.--image-max-tiles